Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[For 10.4] Add share indicator in webUI #36572

Merged
merged 2 commits into from
Dec 19, 2019
Merged

[For 10.4] Add share indicator in webUI #36572

merged 2 commits into from
Dec 19, 2019

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 11, 2019 •
edited by micbar
Loading

Description

This is a rebased and squashed version of #36254 (which has many commits) and then #36534

Filelist shows share indicators on files and folders, that reside inside a shared folder / shared folders.
The sidebar share-tab reveals a detailed view of the share-tree including share-recipients and the respective folders the life in.

2019-11-12-21-23-13

Related Issue

https://github.com/owncloud/enterprise/issues/3453 (Discussion)
https://github.com/owncloud/enterprise/issues/3556 (Vis. Concept)

How Has This Been Tested?

Manual testing in

  • Google Chome
  • Mozilla Firefox
  • MSIE 11

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis phil-davis self-assigned this Dec 11, 2019
@phil-davis phil-davis changed the title Feature/path share info indicator [For 10.4] Add share indicator in webUI Dec 11, 2019
@phil-davis phil-davis mentioned this pull request Dec 11, 2019
Closed
11 tasks
@phil-davis
Copy link
Contributor Author

@individual-it I am happy with the test code (see 2nd commit). Please also give approval if you agree.

@phil-davis
Copy link
Contributor Author

phil-davis commented Dec 11, 2019 •
edited
Loading

IMO this is ready for approve-merge-release

@micbar @PVince81 please allocate someone to review the final run-time code (see the 1st commit) or review it yourself.

(the PR has had some code changes since the original code in the old PRs)

@codecov
Copy link

codecov bot commented Dec 11, 2019 •
edited
Loading

Codecov Report

Merging #36572 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36572      +/-   ##
============================================
+ Coverage     64.68%   64.68%   +<.01%     
  Complexity    19071    19071              
============================================
  Files          1268     1268              
  Lines         74516    74522       +6     
  Branches       1311     1312       +1     
============================================
+ Hits          48201    48207       +6     
- Misses        25929    25930       +1     
+ Partials        386      385       -1
Flag Coverage Δ Complexity Δ
#javascript 54.05% <71.42%> (+0.03%) 0 <0> (ø) ⬇️
#phpunit 65.86% <ø> (ø) 19071 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/share.js 36.97% <ø> (ø) 0 <0> (ø) ⬇️
core/js/sharedialogview.js 78.71% <100%> (+0.1%) 0 <0> (ø) ⬇️
apps/files_sharing/js/share.js 72.44% <100%> (+0.44%) 0 <0> (ø) ⬇️
core/js/apps.js 50% <33.33%> (-1.36%) 0 <0> (ø)
core/js/files/client.js 83.43% <0%> (+0.58%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d74d406...d1b561b. Read the comment docs.

@individual-it
Copy link
Member

tests are fine, we can merge from my point of view

@micbar
Copy link
Contributor

micbar commented Dec 12, 2019

@jnweiger @HanaGemela @michaelstingl @TheOneRing
This is Frontend Only.
It doesn't affect the clients.

So ready to merge, Server QA is fine.

@phil-davis
Copy link
Contributor Author

Waiting on code review and approval of the run-time code.
@micbar @PVince81 who can do that and give a green tick here?

jvillafanez
jvillafanez reviewed Dec 16, 2019
View reviewed changes
apps/files/js/filelist.js Show resolved Hide resolved
apps/files/js/filelist.js Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
core/js/apps.js Show resolved Hide resolved
@phil-davis
Copy link
Contributor Author

@micbar (cc @jvillafanez ) - who is the person who wrote this code in the first place, and/or who will be addressing the comments?
I "own" the PR only because I rebased/squashed.

@felixheidecke
Copy link
Contributor

@phil-davis @jvillafanez I addressed the issues. I took the liberty to directly push the small fixes to this branch.

jvillafanez
jvillafanez reviewed Dec 16, 2019
View reviewed changes
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

Minor remark on #36572 (comment) in case isn't visible (it's marked as outdated, but it's still present).

@phil-davis
Copy link
Contributor Author

phil-davis commented Dec 16, 2019 •
edited
Loading

JS test fails https://drone.owncloud.com/owncloud/core/22017/7/6
and fails again after restart: https://drone.owncloud.com/owncloud/core/22020/7/6
@felixheidecke

@phil-davis
Copy link
Contributor Author

@felixheidecke ping - are you available today?

@phil-davis
Copy link
Contributor Author

@felixheidecke ping again.
This is so close to being merged and we want to be able to make 10.4RC1

@felixheidecke
Copy link
Contributor

Looks like some changes got lost in merging. Changed it back and works now.

@phil-davis
Copy link
Contributor Author

@felixheidecke JS tests failing https://drone.owncloud.com/owncloud/core/22057/7/6

@felixheidecke
Copy link
Contributor

@phil-davis sorry for my weired commits. I'll fix it right away.

@phil-davis
Copy link
Contributor Author

phil-davis commented Dec 18, 2019 •
edited
Loading

This LGTM now. We have had various reviews along the way of code and tests. I squashed the commits.
Can someone now review and hopefully give approval?

@phil-davis phil-davis force-pushed the feature/path-share-info-indicator branch from 6754644 to c36895a Compare December 18, 2019 16:19
@phil-davis
Copy link
Contributor Author

I added a changelog - it was missing

@phil-davis phil-davis force-pushed the feature/path-share-info-indicator branch from a535558 to 289f302 Compare December 19, 2019 05:23
@jvillafanez jvillafanez force-pushed the feature/path-share-info-indicator branch from 480d2fc to 826d8f8 Compare December 19, 2019 08:18
@jvillafanez jvillafanez force-pushed the feature/path-share-info-indicator branch from 826d8f8 to d1b561b Compare December 19, 2019 08:23
@jvillafanez
Copy link
Member

I'll approve once the tests pass.

@phil-davis
Copy link
Contributor Author

I'll approve once the tests pass.

When the JS tests pass then you may as welll approve. Then I can merge as soon as the remaining CI finishes.

@phil-davis phil-davis merged commit 98fc511 into master Dec 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/path-share-info-indicator branch December 19, 2019 09:49
This was referenced Dec 19, 2019
Merged
Merged
@davitol davitol mentioned this pull request Jan 9, 2020
Closed
39 tasks
@PVince81 PVince81 mentioned this pull request Jan 20, 2020
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@individual-it individual-it Awaiting requested review from individual-it

@PVince81 PVince81 Awaiting requested review from PVince81

@micbar micbar Awaiting requested review from micbar

@felixheidecke felixheidecke Awaiting requested review from felixheidecke

Assignees

@phil-davis phil-davis

Labels
4 - To release app:files feature:sharing minor-release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phil-davis @individual-it @micbar @felixheidecke @jvillafanez